Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor test-readline-async-iterators into a benchmark #49237

Merged
merged 2 commits into from
Sep 23, 2023
Merged

Refactor test-readline-async-iterators into a benchmark #49237

merged 2 commits into from
Sep 23, 2023

Conversation

shubham9411
Copy link
Contributor

Refactor test-readline-async-iterators into a benchmark
Fixes: #49224
Added old way of readline async iterator to benchmark.

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. needs-ci PRs that need a full CI run. readline Issues and PRs related to the built-in readline module. test Issues and PRs related to the tests. labels Aug 19, 2023
@shubham9411
Copy link
Contributor Author

Running the benchmark on my system produces following results:

readline/readline-iterable.js type="old" n=10: 18,494.459304547192
readline/readline-iterable.js type="new" n=10: 30,235.50963200886
readline/readline-iterable.js type="old" n=100: 103,827.61676284338
readline/readline-iterable.js type="new" n=100: 225,055.64552367883
readline/readline-iterable.js type="old" n=1000: 540,078.7387188078
readline/readline-iterable.js type="new" n=1000: 930,493.4352180096
readline/readline-iterable.js type="old" n=10000: 1,620,921.425907833
readline/readline-iterable.js type="new" n=10000: 2,710,950.150289306
readline/readline-iterable.js type="old" n=100000: 2,205,084.645319321
readline/readline-iterable.js type="new" n=100000: 3,754,004.952936157
readline/readline-iterable.js type="old" n=1000000: 2,301,016.7856763634
readline/readline-iterable.js type="new" n=1000000: 4,040,921.510293063

mcollina
mcollina approved these changes Aug 19, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 19, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 19, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53442/

@shubham9411
Copy link
Contributor Author

I see two tests failed. For commit message should I change commit and push or it is ok and will be changed while merging?
For macOS test I think it is a flaky test but not sure 🤔 .

@github-actions github-actions bot mentioned this pull request Aug 21, 2023
Open
35 tasks
@shubham9411 shubham9411 mentioned this pull request Aug 21, 2023
Closed
@github-actions github-actions bot mentioned this pull request Aug 22, 2023
Open
21 tasks
@shubham9411
Copy link
Contributor Author

Hi @mcollina, sorry you may be busy, just wanted to know if there are any changes needed from my side on this.

@github-actions github-actions bot mentioned this pull request Aug 23, 2023
Open
27 tasks
@mertcanaltin
Copy link
Member

@shubham9411

Hello, your first commit message has an incorrect prefix; it can be formatted as follows:

test: refactor test-readline-async-iterators into a benchmark

@shubham9411 shubham9411 force-pushed the shubham9411/readline-iterable-benchmark branch from 6a7950d to 429e78d Compare August 23, 2023 13:36
@shubham9411
Copy link
Contributor Author

@mertcanaltin thanks, I have updated

@github-actions github-actions bot mentioned this pull request Aug 24, 2023
Open
27 tasks
@shubham9411 shubham9411 requested a review from mcollina September 1, 2023 16:47
@shubham9411
Copy link
Contributor Author

I am not sure why 3 of the tests are stuck in InProgress state. Can someone help?

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2023
@mcollina
Copy link
Member

mcollina commented Sep 4, 2023

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1365/

mcollina
mcollina approved these changes Sep 4, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53714/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53721/

@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 4, 2023
@shubham9411
Copy link
Contributor Author

Logs from test-tarball-linux suggests that all tests succeeded. But it seems to be stuck 🤔 https://github.com/nodejs/node/actions/runs/5952088308/job/16430260251

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 7, 2023
@github-actions github-actions bot mentioned this pull request Sep 21, 2023
Open
22 tasks
@mcollina
Copy link
Member

@nodejs/build @nodejs/testing could somebody take a look at this? It keeps failing but there are less tests, not more tests, so they should not be.

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54118/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54135/

@github-actions github-actions bot mentioned this pull request Sep 22, 2023
Open
17 tasks
@richardlau
Copy link
Member

@nodejs/build @nodejs/testing could somebody take a look at this? It keeps failing but there are less tests, not more tests, so they should not be.

I started a new CI -- the only failure was a temporary infra issue while we've been adding gcc 10 to the test machines (this particular failure fixed in nodejs/build#3495). One resume CI later and the Jenkins CI has passed for this PR.

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. needs-ci PRs that need a full CI run. labels Sep 22, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 22, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/49237
✔  Done loading data for nodejs/node/pull/49237
----------------------------------- PR info ------------------------------------
Title      Refactor test-readline-async-iterators into a benchmark (#49237)
Author     Shubham Pandey  (@shubham9411)
Branch     shubham9411:shubham9411/readline-iterable-benchmark -> nodejs:main
Labels     readline, test, benchmark, author ready, commit-queue-squash
Commits    2
 - test: refactor test-readline-async-iterators into a benchmark
 - empty commit
Committers 1
 - Shubham Pandey 
PR-URL: https://github.com/nodejs/node/pull/49237
Fixes: https://github.com/nodejs/node/issues/49224
Reviewed-By: Matteo Collina 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/49237
Fixes: https://github.com/nodejs/node/issues/49224
Reviewed-By: Matteo Collina 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - test: refactor test-readline-async-iterators into a benchmark
   ⚠  - empty commit
   ℹ  This PR was created on Sat, 19 Aug 2023 05:18:29 GMT
   ✔  Approvals: 1
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/49237#pullrequestreview-1609041171
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-09-21T17:12:52Z: https://ci.nodejs.org/job/node-test-pull-request/54135/
   ℹ  Last Benchmark CI on 2023-09-07T09:20:49Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1365/
- Querying data for job/node-test-pull-request/54135/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/6274851210

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 22, 2023
@github-actions github-actions bot mentioned this pull request Sep 23, 2023
Open
16 tasks
@shubham9411
Copy link
Contributor Author

Hey @mcollina, I pushed one empty commit just to trigger the CI pipeline. Do I have to remove it as commit queue is failing. Thanks

@mcollina mcollina removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 23, 2023
mcollina
mcollina approved these changes Sep 23, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 23, 2023
@mcollina
Copy link
Member

No need

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 23, 2023
@nodejs-github-bot nodejs-github-bot merged commit c0b4208 into nodejs:main Sep 23, 2023
18 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in c0b4208

@github-actions github-actions bot mentioned this pull request Sep 24, 2023
Open
20 tasks
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
@shubham9411 @ruyadorno
test: refactor test-readline-async-iterators into a benchmark
3db9b40 
PR-URL: #49237
Fixes: #49224
Reviewed-By: Matteo Collina <[email protected]>
This was referenced Sep 28, 2023
Closed
Merged
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@shubham9411 @alexfernandez
test: refactor test-readline-async-iterators into a benchmark
8ba50b8 
PR-URL: nodejs#49237
Fixes: nodejs#49224
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. benchmark Issues and PRs related to the benchmark subsystem. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. readline Issues and PRs related to the built-in readline module. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor test-readline-async-iterators into a benchmark
5 participants
@shubham9411 @nodejs-github-bot @mertcanaltin @mcollina @richardlau